Decoupling ActiveSupport from ActionView #6526
Conversation
| module ActiveSupport | ||
| module Testing | ||
| module Performance | ||
| class PerformanceTest < ActiveSupport::TestCase |
There was a problem hiding this comment.
I think there's no need for the Performance module, wdyt?
|
Other than that, looks 👍 to me, thanks! |
|
I needed some module because I didn't want the class name It works just as well without it however, i'll update the PR to remove the Thanks for the feedback! |
|
Sure, your idea is definitely a good logic to follow, I agree about having some module namespacing in such scenarios. I just thought that having both |
|
Ok, I have removed the extra module and squashed it. Let me know if you'd like any other changes. :) |
| assert_equal "-5000 ms", time_metric.format(-5) | ||
| end | ||
|
|
||
| def test_amount_time |
There was a problem hiding this comment.
I just noticed both tests are named the same test_amount_time :)
|
Also, it seems there's a missing require (totally unrelated, but lets take the opportunity): I get this when running that single file: Thanks. |
|
Great catch on the methods names! I've fixed them. I've also added the missing require and the file loads fine now individually. Let me know if you'd like any other changes. Thanks! |
|
Looking great, thank you. |
…actionivew Decoupling ActiveSupport from ActionView
|
This is awesome! 👍 👍 👍 👍 |
Hi,
I'm interested in using the ActiveSupport::Testing::Performance module and was surprised to see that it requires and calls code from ActionView. I'd like to use this module in a context without ActionView, and now that we've moved NumberHelpers to ActiveSupport (#6315), we can do this.
In this pull request I'm changing ActiveSupport::Testing::Performance to use ActiveSupport::NumberHelper instead of ActionView::Helpers::NumberHelper. Also, to my knowledge this logic was untested, so I have added tests for the formatting logic (which pass before and after the code change).
Any and all feedback is welcome.
Thanks!
-Andrew.